Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A way to configure a cluster wide queue limit #11196

Merged
merged 1 commit into from
May 22, 2024

Conversation

SimonUnge
Copy link
Member

@SimonUnge SimonUnge commented May 8, 2024

Proposed Changes

Much simpler approach to #10748.

Version for main: #11212.

This solution uses a limit conf setting.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc.

@SimonUnge SimonUnge changed the base branch from main to v3.13.x May 8, 2024 21:23
@SimonUnge
Copy link
Member Author

SimonUnge commented May 8, 2024

@michaelklishin @mkuratczyk this is a simpler approach.

Another way to 'store' the cluster wide queue limit, instead of config, would perhaps be to store it as a global parameter?
i.e rabbitmqctl set_global_parameter cluster_limit '{"queue":"5"}'

Or, read the config at start and store the value in a db, and let it be overwritten by whatever node writes last?

Personally I don't think having a 'cluster setting' per node is that much of an issue, but am willing to change my mind quickly.

@michaelklishin michaelklishin changed the title Cluster wide queue limit A way to configure a cluster wide queue limit May 8, 2024
@michaelklishin
Copy link
Member

michaelklishin commented May 8, 2024

No, we should not use global parameters for things that are not dynamic in nature, and knowing the background on this, this is something that an absolute majority of installations will set in stone and rarely change, perhaps after a period of adaptation. Or not set in the first place.

@SimonUnge
Copy link
Member Author

OK, ill leave as is and add some tests.

@michaelklishin
Copy link
Member

michaelklishin commented May 8, 2024

@SimonUnge as for setting this cluster-wide setting on every node, since most cluster provisioning tools/options perform config generation one way or another, this won't be an issue. As long as the value is identical on all nodes anyway, which we can assume to be the case most of the time and/or eventually after a redeployment.

@SimonUnge
Copy link
Member Author

yes, I agree. Its the easiest and most straightforward approach to setting it.

@mkuratczyk
Copy link
Contributor

Thanks Simon. The limit is enforced in the wrong place however - it's only applicable to AMQP 0.9.1 and "non-native" protocols, so that'd be just STOMP in 4.0 or perhaps not even that. I can easily declare more queues than the limit I set by:

  • importing definitions
  • starting MQTT subscriptions
  • using stream-perf-test
    and probably a few other ways

I think these checks should be moved to rabbit_queue_type:declare

@SimonUnge
Copy link
Member Author

SimonUnge commented May 9, 2024

@mkuratczyk ah right you are, I'll move it (I basically just placed it where the vhost queue limit is). I'll move it to a better place.

I will not move the vhost limit in this PR to keep it clean, but can create an issue for it.

@mergify mergify bot added the bazel label May 9, 2024
@SimonUnge SimonUnge changed the base branch from v3.13.x to main May 10, 2024 16:32
@SimonUnge SimonUnge changed the base branch from main to v3.13.x May 10, 2024 16:33
@SimonUnge
Copy link
Member Author

@michaelklishin noticed I pointed this one to v3.13.x... Should I switch to main?

@michaelklishin
Copy link
Member

You need to create a new branch off of main, cherry-pick the commit(s) there and push it, then submit a PR against main.

@SimonUnge SimonUnge changed the base branch from v3.13.x to main May 10, 2024 17:08
@mergify mergify bot added the make label May 10, 2024
@SimonUnge SimonUnge changed the base branch from main to v3.13.x May 10, 2024 17:10
@SimonUnge SimonUnge mentioned this pull request May 10, 2024
12 tasks
@SimonUnge
Copy link
Member Author

@michaelklishin Done. Also squashed this commit.

@SimonUnge
Copy link
Member Author

Will rebase on #11222 once merged, to combine the logic a bit.

@SimonUnge SimonUnge force-pushed the cluster_wide_queue_limit branch 3 times, most recently from ee526da to 50c39b5 Compare May 21, 2024 20:44
@michaelklishin michaelklishin merged commit cf8d73d into rabbitmq:v3.13.x May 22, 2024
11 checks passed
@michaelklishin michaelklishin added this to the 3.13.3 milestone May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants